COM Record field assignment using type information and multidim SAFEARRAYs as field values#2655
Conversation
…Y(double) Create SAFEARRAY(double) from a sequence of PyFloat objects instead of SAFEARRAY(VARIANT(RT_8)). The implementation builds multidimensional SAFEARRAYs from nested sequences. The creation of SAFEARRAY(VT_RECORD) was improved to also allow nested sequences.
|
@Avasam I had to retarget the Visual Studio project of PyCOMTest for the build tools v143 with a wild card for "the latest" installed Win SDK version because the build did fail, complaining that:
|
I don't mind the retargeting and bumping the platform toolset to v143 (Visual Studio 2022) makes sense. I probably missed that somewhere in the recent CI migration to As for the rest of the PR, I am not qualified to review that / it is past my areas of expertise, so 🤷 Leaving to @mhammond |
Resolve an inefficiency
mhammond
left a comment
There was a problem hiding this comment.
Thanks - I'm going to need more time to look at this thoroughly, but some initial thoughts...
9077722 to
6617885
Compare
|
After reviewing the code path for the assignment of values to Record fields, I think the problem is that it was wrong from the beginning. When I implemented the creation of SAFEARRAY(VT_RECORD) from a sequence of COM Records #2317, I followed the existing code path into Obviously IDL files in the field could contain The culprit is the I think I have now reverted my changes in To fulfill the type requirements defined in an IDL file for the @mhammond Please let me know if you agree with this in general. |
|
@mhammond Apart from the details, are you in general OK with the revised approach for setting COM Record field values? |
|
Is there anything I can do to move this PR forward? While I understand that reviewing this PR is not a 5 minute task, not getting a single comment over the past 4 months is a little bit disappointing. If some of the changes need more explanation or if you disagree with the general approach please let me know. |
mhammond
left a comment
There was a problem hiding this comment.
I'm very sorry about the delay here, but this seems good to me, and the tests are exhaustive - thanks, and again, sorry for letting this slip.
|
Thanks. I can completely understand that larger PRs do take some time to get reviewed. Please, in the future don't hesitate to just reply with a "sorry, I currently don't have the bandwidth to deal with this, please give me a ping in about one or two months". You did this in your first comment September last year but then it went completely quiet which made me feel a little lost. |
|
Thanks! But the sad reality is just that I simply lost track of it, there were simply sitting as unread emails in my inbox. There are many PRs open, many of which I'm really quite ambivalent about, and sorting the wheat from the chaff is something I need to get better about and address. fwiw, please do feel free to ping me when I slip up. |
|
Uhh, emails. Github is already flooding my inbox with messages from the few repos I'm watching. The problem is that you get a message for each and every comment and commit. The Github notifications at least list the Issues/PRs that had changes as a summary. @Avasam did create the Bookmarkable filters to reduce noise and help prioritize PRs in the Discussions which can probably help you sort the chaos. |
This PR implements:
These do also cover a test case as requested in Bugfix for COM Record instance creation. #2641 (review).